Skip to content

PHPLIB-70: Resource CRUD prototypes #5

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
Mar 18, 2015

Conversation

jmikola
Copy link
Member

@jmikola jmikola commented Mar 18, 2015

* @param string $databaseName
* @return Result
*/
public function dropDatabase($databaseName)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the Database object have __toString() method and this code explicitly do (string)$databaseName ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we did that and allowed the argument to be either a string or a Database instance, would we get into trouble with dropCollection()? I would expect a Collection's __toString() method to return its full namespace, as we do in the 1.x driver. I'd rather not invite the inconsistency.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The proto should definetly not be string|Database, it should be string - but if it does (string) casting inside the method it implicitly allows the Database object to work seamlessly.

as for dropCollection() returning fully qualified namespace...... mmmmh.....

We can move this to a separate, pedantic test class later.
* @return Result
*/
public function drop()
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens to this instance after calling drop?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

exec('halt -f');
exit(0);

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But really, we don't need to do anything. A Collection instance doesn't require anything to actually exist in the database, so there's nothing to clean up in PHP. If we started doing stuff like caching index enumeration class, perhaps, but not now.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$collection = $db->createCollection("foo", array("storageEngineOptions" => array("xyz"));
$collection->insert(array("foo" => "bar"));
$collection->drop();
$collection->insert(array("foo" => "bar")); // This will create different configured collection then above

@bjori
Copy link
Contributor

bjori commented Mar 18, 2015

lgtm

@bjori
Copy link
Contributor

bjori commented Mar 18, 2015

Do we now how stubs for the entire API we want before the alpha release?

@jmikola
Copy link
Member Author

jmikola commented Mar 18, 2015

@bjori: I left out collMod and there are a few other tickets for things like DBRef we should probably include. Let's chat later today.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants